Add ability to disable auto trigger deployment on git change by khanhtc1202 · Pull Request #2714 · pipe-cd/pipecd

khanhtc1202 · 2021-10-28T10:20:51Z

What this PR does / why we need it:

Sample configuration for this looks like

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  trigger:
    onCommit:
       disable: true
  input:
    ...

Besides, we can also configure watching paths via spec.trigger.onCommit.Paths field, the previous spec.triggerPaths is marked as deprecated and will be removed later.

Which issue(s) this PR fixes:

Fixes #2245

Does this PR introduce a user-facing change?:

Add ability to disable auto-trigger deployment on Git changes

pipecd-bot · 2021-10-28T10:28:19Z

Code coverage for golang is 32.13%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-10-28T11:31:35Z

Code coverage for golang is 32.13%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-10-28T11:47:20Z

Code coverage for golang is 32.13%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-10-29T05:27:20Z

Code coverage for golang is 32.13%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

nghialv · 2021-10-29T05:39:45Z

pkg/config/deployment.go

 	SealedSecrets []SealedSecretMapping `json:"sealedSecrets"`
-	// List of directories or files where their changes will trigger the deployment.
-	// Regular expression can be used.
-	TriggerPaths []string `json:"triggerPaths,omitempty"`


We should keep both of fields to avoid breaking changes.

Ah, I see, then mark it as deprecated and remove it later 👍 Lets me address this

Addressed by 6591559

better update our docs as well

Sure, will do it by a separated PR 🙏

pipecd-bot · 2021-10-29T06:07:07Z

Code coverage for golang is 32.12%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pkg/app/piped/trigger/determiner.go

nghialv · 2021-10-29T06:44:37Z

This should fix #2245

nghialv · 2021-10-29T06:48:05Z

pkg/config/deployment.go

+	// Regular expression can be used.
+	Paths []string `json:"paths,omitempty"`
+	// Control trigger new deployment on Git change or not.
+	DisableAutoDeployOnChange bool `json:"disableAutoDeployOnChange,omitempty"`


How about:

triggerOnChange: true triggerOnSyncCommand: true triggerOnChain: true

or

onChange: true onSyncCommand: true onChain: true

or

on: - GIT_CHANGE - SYNC_COMMAND - CHAIN

or

onChange: disabled: true onSyncCommand: disabled: true onChain: disabled: true otherConfig: foo

I get your point; let me carefully think about this 👍 At a glance, I believe (3) is best for our deployment chain implementation.

👍 Let take the time to think about that.

Just added another option.

That's too many (but I liked it) 😄

khanhtc1202 · 2021-10-29T07:35:48Z

/hold

pipecd-bot · 2021-11-05T10:16:06Z

Code coverage for golang is 32.10%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-11-08T08:55:46Z

Code coverage for golang is 32.10%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-11-08T09:36:17Z

Code coverage for golang is 32.09%. This pull request decreases coverage by -0.02%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

khanhtc1202 · 2021-11-08T10:19:40Z

/hold cancel

pipecd-bot · 2021-11-08T10:24:31Z

Code coverage for golang is 32.08%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.shouldTriggerOnCommit	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-11-08T10:33:46Z

Code coverage for golang is 32.08%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.shouldTriggerOnCommit	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

nghialv · 2021-11-09T03:31:31Z

pkg/config/deployment.go

+
+type OnCommitConfig struct {
+	// Control trigger new deployment on Git change or not.
+	Disable bool `json:"disable,omitempty"`


"Disabled" because we are using the same format at other places.

nit: Do we need the Config suffix?

Addressed by b9bc4cc

nghialv · 2021-11-09T03:33:34Z

pkg/config/deployment.go

 	SealedSecrets []SealedSecretMapping `json:"sealedSecrets"`
 	// List of directories or files where their changes will trigger the deployment.
 	// Regular expression can be used.
+	// Deprecated: use Trigger.Paths instead.


The docs in dev should be updated too.

Will do in a separated PR 🙆‍♀️

nghialv · 2021-11-09T03:34:43Z

pkg/config/deployment.go

+
+type OnCommitConfig struct {
+	// Control trigger new deployment on Git change or not.
+	Disable bool `json:"disable,omitempty"`


nit: Do we need the Config suffix?

nghialv · 2021-11-09T03:40:39Z

pkg/app/piped/trigger/determiner.go

-func (d *Determiner) ShouldTrigger(ctx context.Context, app *model.Application) (bool, error) {
+// Flag `ignoreUserConfig` set to `true` will force check changes and use it to determine
+// the application deployment should be triggered or not, regardless of the user's configuration.
+func (d *Determiner) ShouldTrigger(ctx context.Context, app *model.Application, ignoreUserConfig bool) (bool, error) {


How about moving ignoreUserConfig to Determiner while initializing it? In that way, we can keep this function simple as it is.

Sure, lets me adopt that 🙆‍♀️

addressed by 8dffc8d

pipecd-bot · 2021-11-09T04:24:46Z

Code coverage for golang is 32.08%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.shouldTriggerOnCommit	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-11-09T04:57:17Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Add logic to determine trigger or not based on other configuration than onCommit.

https://github.com/pipe-cd/pipe/blob/8dffc8dde1ce421ecde90d24f915b6589f3b63db/pkg/app/piped/trigger/determiner.go#L64-L67

This was created by todo plugin since "TODO:" was found in `8dffc8d` when #2714 was merged. cc: @khanhtc1202.

2. Remove deprecated `deployConfig.TriggerPaths` configuration.

https://github.com/pipe-cd/pipe/blob/8dffc8dde1ce421ecde90d24f915b6589f3b63db/pkg/app/piped/trigger/determiner.go#L107-L110

This was created by todo plugin since "TODO:" was found in `8dffc8d` when #2714 was merged. cc: @khanhtc1202.

pipecd-bot · 2021-11-09T05:04:31Z

Code coverage for golang is 32.08%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/app/piped/trigger/determiner.go	Determiner.shouldTriggerOnCommit	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`0.00%`	`+0.00%`

nghialv · 2021-11-09T06:44:07Z

Great!
/lgtm

nakabonne · 2021-11-09T06:51:39Z

This small step is the first step of a great walk. Well done 👍
/approve

pipecd-bot · 2021-11-09T06:51:42Z

This pull request is APPROVED by nakabonne.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

pipecd-bot added do-not-merge/release-note-needed area/piped do-not-merge/wip size/M labels Oct 28, 2021

pipecd-bot requested review from nakabonne and ono-max October 28, 2021 10:20

pipecd-bot added release-note-none and removed do-not-merge/release-note-needed labels Oct 28, 2021

pipecd-bot removed the release-note-none label Oct 29, 2021

khanhtc1202 marked this pull request as ready for review October 29, 2021 05:28

pipecd-bot removed the do-not-merge/wip label Oct 29, 2021

nghialv reviewed Oct 29, 2021

View reviewed changes

ono-max reviewed Oct 29, 2021

View reviewed changes

pkg/app/piped/trigger/determiner.go Outdated Show resolved Hide resolved

nghialv reviewed Oct 29, 2021

View reviewed changes

pipecd-bot added the do-not-merge/hold label Oct 29, 2021

khanhtc1202 added 5 commits November 5, 2021 19:08

Add ability to disable auto trigger deployment on git change

dce2482

Change flag name

0fdd89e

Remove log

f02c940

Add ignorable flag

7b5c119

Keep deployConfig.TriggerPaths as deprecated configuration

cbd67d8

khanhtc1202 force-pushed the add-disable-auto-trigger-deploy branch from 6591559 to cbd67d8 Compare November 5, 2021 10:08

Fix configuration

d8899a9

Update tests and rename should trigger ignore flag

148765f

pipecd-bot added size/L and removed size/M labels Nov 8, 2021

remove empty string path

471d092

pipecd-bot removed the do-not-merge/hold label Nov 8, 2021

Remove unnecessary code

5734d05

nghialv reviewed Nov 9, 2021

View reviewed changes

Rename

b9bc4cc

Move ignoreUserConfig flag to Determiner struct

8dffc8d

pipecd-bot added the lgtm label Nov 9, 2021

pipecd-bot added the approved label Nov 9, 2021

pipecd-bot merged commit 9fe9af6 into master Nov 9, 2021

pipecd-bot deleted the add-disable-auto-trigger-deploy branch November 9, 2021 07:00

This was referenced Nov 9, 2021

Add logic to determine trigger or not based on other configuration than onCommit. #2761

Closed

Remove deprecated deployConfig.TriggerPaths configuration. #2762

Closed

This was referenced Dec 1, 2021

Update doc for deployment triggering #2867

Merged

Temporarily disable appconfig reporter in Piped #2868

Merged

Conversation

khanhtc1202 commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pipecd-bot commented Oct 28, 2021

Uh oh!

pipecd-bot commented Oct 28, 2021

Uh oh!

pipecd-bot commented Oct 28, 2021

Uh oh!

pipecd-bot commented Oct 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Oct 29, 2021

Uh oh!

Uh oh!

nghialv commented Oct 29, 2021

Uh oh!

nghialv Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 commented Oct 29, 2021

Uh oh!

pipecd-bot commented Nov 5, 2021

Uh oh!

pipecd-bot commented Nov 8, 2021

Uh oh!

pipecd-bot commented Nov 8, 2021

Uh oh!

khanhtc1202 commented Nov 8, 2021

Uh oh!

pipecd-bot commented Nov 8, 2021

Uh oh!

pipecd-bot commented Nov 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Nov 9, 2021

Uh oh!

pipecd-bot commented Nov 9, 2021

1. Add logic to determine trigger or not based on other configuration than onCommit.

This was created by todo plugin since "TODO:" was found in 8dffc8d when #2714 was merged. cc: @khanhtc1202.

2. Remove deprecated deployConfig.TriggerPaths configuration.

This was created by todo plugin since "TODO:" was found in 8dffc8d when #2714 was merged. cc: @khanhtc1202.

Uh oh!

pipecd-bot commented Nov 9, 2021

khanhtc1202 commented Oct 28, 2021 •

edited

Loading

nghialv Oct 29, 2021 •

edited

Loading

This was created by todo plugin since "TODO:" was found in `8dffc8d` when #2714 was merged. cc: @khanhtc1202.

2. Remove deprecated `deployConfig.TriggerPaths` configuration.

This was created by todo plugin since "TODO:" was found in `8dffc8d` when #2714 was merged. cc: @khanhtc1202.